@tvdeyen thanks for the early heads up!

I really don't like to add this amount of code into every store that gets created out there. Could you elaborate why this change is necessary and maybe discuss another way of fixing those issues?

I'll defer to the section below for explaining how this is necessary for the current subscriber implementation (AS::Notifications) and most probably for alternative ones. I also agree that keeping that amount of code there is not ideal. What about leaving just the glob and subscribe with an internal API? E.g.:

Dir.glob(Rails.root.join('app/subscribers/*_subcriber.rb)) do |path|
  Spree::Event.register_subscriber(path)
end

Alternatively we can also define the glob in a configuration, e.g. Spree::Config.events.files = Dir.glob(Rails.root.join('app/subscribers/*_subcriber.rb)), and do everything else internally.

From what I can tell in the moment this looks like an implemenation issue we have with the current subscribers implementation, right? If so, could we try to fix those instead?

That's correct, namely the problem is with AS::Notifications, although, that's likely something that will be needed for any other subscriber implementation that organizes subscribers in different classes/modules across different files (which seems a pretty standard way of doing it). And allowing an implementation to provide a list of constant-names to be re-subscribed at each reload seems a general-enough api.

To better exemplify the problem I'll write some psedo-code that will hopefully make it clear for anyone not super-familiar with rails reloading, the following is based on the current implementation / examples.

I apologize if this is known stuff, but I thought it might be a useful refresher for some 🙂

Scenario 1: loading subscribers with an initializer

The app starts and subscribes all subscribers

# config/initializers/spree_event_subscribers.rb
OrderSubscriber.subscribe! # Rails will load app/subscribers/order_subscriber.rb
PaymentSubscriber.subscribe! # Rails will load app/subscribers/payment_subscriber.rb

Rails will load the missing constants in this way

unless const_defined? :OrderSubscriber
  load 'app/subscribers/order_subscriber.rb'
  mark_as_autoloaded :OrderSubscriber, 'app/subscribers/order_subscriber.rb'
  p OrderSubscriber # => #<Module:123>
end

AS::Notification will store the contents of OrderSubscriber inside a global list of subscribers

AS::Notification.subscribers << OrderSubscriber # that's an instance of Module: #<Module:123>

The developer changes the contents of app/subscribers/order_subscriber.rb

if file_changed? 'app/subscribers/order_subscriber.rb'
  remove_const :OrderSubscriber
end

# at this point AS::Notification still holds a reference to the old version of OrderSubscriber  
p AS::Notification.subscribers # => [#<Module:123>, …]

The new version of OrderSubscriber is never loaded since the constant is not referenced anywhere except in the initializer

Scenario 2: loading subscribers with to_prepare

Goes like scenario 1, but order_subscriber.rb is loaded within the to_prepare block which is called at initialization and after each application reload

When the app reloads the constant is removed as above, and redefined with the new implementation of OrderSubscriber, let's call it #Module:456, which is a completely different instance of Module

The problem remains with the global AS::Notifications global list that still holds the old reference, plus the new reference: [#<Module:123>, …, #<Module:456>, …]

The result in this case is that we have duplicate subscribers with different implementations

We are discussing this IRL and we decided to try to use an approach similar to calculators so we'll probably end up having a list of subscribers classes into a preference like

spree.config.event.subscribers = [
  Spree::Core::Event::MailSubscriber,
  SpreeExtensions::AnotherSubscriber,
  MyApp::YetAnotherSubscriber
]

This looks like a good solution for a couple of reasons:

1. it is something easy to manage to add change or remove classes in the list
2. it is a well-known pattern for Solidus users since we are using it in other places. If we find a more Rails-way solution for this kind of object we can easily replace everything and deprecate setting the configuration.

@kennyadsl @tvdeyen ready for a new (final?) pass 🔍🙂